Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed compiling of constructor and type reference. Fixed unary plus/minus. Added node.getRaw(). #14

Merged
merged 8 commits into from
Jan 21, 2022
Merged

Fixed compiling of constructor and type reference. Fixed unary plus/minus. Added node.getRaw(). #14

merged 8 commits into from
Jan 21, 2022

Conversation

ukrbublik
Copy link
Contributor

@ukrbublik ukrbublik commented Jan 13, 2022 •
edited
Loading

  1. Fixed error Cannot read property 'setParent' of undefined when trying to compile expression with constructor or type reference
new java.text.SimpleDateFormat("yyyy-MM-dd").parse("2022-01-01")
T(java.time.LocalTime).parse("11:22")
  1. Added support for array constructors
new int[]{1, 2, 3}
new int[3]{1, 2, 3}

  1. Fixed urary plus/minus. Resolves issue Expression with negative value failed to compile #12

  2. Added node.getRaw() for some types to get raw value (eg. propertyName for property node).
    Can be used for debugging compiled expressions.
    Used in https://github.com/ukrbublik/react-awesome-query-builder to convert compiled expression to internal store format.

Added unit tests

@ukrbublik ukrbublik mentioned this pull request Jan 13, 2022
Merged
benmarch
benmarch reviewed Jan 20, 2022
View reviewed changes
Copy link
Owner

@benmarch benmarch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ukrbublik sorry for the delayed response, but thank you for this PR!

I have just a few minor requests and a question for you, otherwise this looks good to go.

src/ast/OpMinus.js Outdated Show resolved Hide resolved
src/ast/ConstructorReference.js
Comment on lines 75 to 78
throw {
name: 'MethodNotImplementedException',
message: 'BeanReference: Not implemented'
message: 'ConstructorReference: Not implemented'
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this throw be removed now since it's implemented? It looks like it's unreachable anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's implemented only for array construction:

spel2js/src/ast/ConstructorReference.js

Line 51 in f5b05db

if (isArray && args.length <= 1) {

For other classes it will throw ConstructorReference: Not implemented (just renamed typo here BeanReference -> ConstructorReference )

src/ast/ConstructorReference.js
if (args.length == 1) {
return compiledArgs[0];
} else {
return dimension ? new Array(dimension) : [];
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return dimension ? new Array(dimension) : [];
return dimension ? [dimension] : [];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line would create an empty array with dimension, not an array with 1 element.

Example in console:

> new Array(4)
 Array(4) [ <4 empty slots> ]
  length: 4

> [3]
 Array [ 3 ]
  0: 3
  length: 1

See test for evaluating new int[3]:

spel2js/test/spec/SpelExpressionEvaluator.spec.js

Lines 906 to 918 in f5b05db

it('should create new empty array with dimension', ()=>{
//given
let context = {};
//when
let newArray = evaluator.eval('new int[3]', context);
//then
expect(newArray.length).toEqual(3);
});
});
});

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, thanks

src/ast/ConstructorReference.js Outdated Show resolved Hide resolved
@benmarch
Copy link
Owner

Thanks, @ukrbublik, looks good to me. I'll publish a new version with these changes shortly.

@benmarch benmarch merged commit ae83580 into benmarch:master Jan 21, 2022
@ukrbublik
Copy link
Contributor Author

Thanks @benmarch 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmarch benmarch Awaiting requested review from benmarch

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ukrbublik @benmarch